-
Notifications
You must be signed in to change notification settings - Fork 15
[WIP] createContexts #325
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: next
Are you sure you want to change the base?
[WIP] createContexts #325
Conversation
Deploying with
|
Status | Name | Latest Commit | Preview URL | Updated (UTC) |
---|---|---|---|---|
✅ Deployment successful! View logs |
synapse-dev | 32e108b | Commit Preview URL Branch Preview URL |
Oct 18 2025, 03:19 AM |
return await StorageContext.createWithSelectedProvider(resolution, synapse, warmStorageService, options) | ||
} | ||
|
||
static async createWithSelectedProvider( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we're exposting this, we're essentially exposing ProviderSelectionResult
as a public type that someone might use. So we should look at this flow a little more critically.
- We use
-1
to signal that a new dataSet needs to be made, but we could also just omit it for that case I think. - The type has a bizzaro
isNewDataSet
that's not used anywhere. Let's remove that. isExisting
is only used for the callback, but also is now overloading our use of-1
indataSetId
, so we don't really need it now either- The type really is a combination of Provider+DataSet (which is what a Context is supposed to be structured around), so how about we rename it.
Here's an interesting option, make it an "intent" for how we want the context to be:
interface StorageContextIntent {
provider: ProviderInfo
dataSetIntent:
| { action: 'create', metadata: Record<string, string> }
| { action: 'reuse', id: number, metadata: Record<string, string> }
}
Simple variation that's not to different to now:
interface ProviderDataSetSelection {
provider: ProviderInfo
dataSetId?: number // undefined = needs creation
dataSetMetadata: Record<string, string>
}
But I think I like the explicit descriptiveness of an intent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was planning to make this method private
Reviewers @rvagg @hugomrdias
Context
Toward #312
Changes